Skip to content

Conversation

felipecsl
Copy link
Contributor

@felipecsl felipecsl commented Dec 4, 2024


labels: mergeable

Fixes: #issue

Motivation and Context

Fix process is not defined error

Description

Eppo-exp/js-sdk-common#136 introduced a change to applicationLogger that added a check for process.env.LOG_LEVEL.
This doesn't work on the browser because process only exists in the Node environment.
Although there's a tricky reason why the original (process.env.NODE_ENV === 'production' ? 'warn' : 'info') expression didn't already fail, which is that webpack resolves the entire expression to simply warn since it resolves process.env.NODE_ENV to production at build time.
I've worked around it by doing a similar preprocessing step for LOG_LEVEL by adding a webpack.DefinePlugin to the webpack config that sets it to null.
This causes webpack to simplify the entire line to just level: "warn".

How has this been tested?

yarn webpack
Inspect the generated file dist/eppo-sdk.js. It should have a line line like below:

t.loggerPrefix = "[Eppo SDK]", t.logger = (0, n.default)({level: "warn", browser: {disabled: !0}})

Create a index.html file with the following contents:

<script src="file:///absolute/path/to/js-client-sdk/dist/eppo-sdk.js"></script>
<script>
    console.log(window.eppo);
</script>

Load this page in the browser and there should be no errors in the dev tools console.

@felipecsl felipecsl changed the title fix: Set process.env.LOG_LEVEL to null in webpack config fix: Set process.env.LOG_LEVEL to null in webpack config Dec 4, 2024
Copy link
Member

@leoromanovsky leoromanovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for testing thoroughly

Copy link
Contributor

@sameerank sameerank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work investigating! Do you want to patch the version in this PR too to publish the fix?

Copy link
Collaborator

@greghuels greghuels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-12-04 at 4 58 56 AM

I'm still getting Buffer is not defined, so I don't think this is the full solution. It looks like it's coming from https://github.com/Eppo-exp/js-sdk-common/blob/0587b36dc4547a29fbfd351ad3a46954d3263684/src/events/sdk-key-decoder.ts#L10. I think we need to update that file with the decodeBase64 function in src/obfuscation.ts.

@felipecsl
Copy link
Contributor Author

Screenshot 2024-12-04 at 4 58 56 AM I'm still getting Buffer is not defined, so I don't think this is the full solution. It looks like it's coming from https://github.com/Eppo-exp/js-sdk-common/blob/0587b36dc4547a29fbfd351ad3a46954d3263684/src/events/sdk-key-decoder.ts#L10. I think we need to update that file with the `decodeBase64` function in `src/obfuscation.ts`.

We already added a polyfill for buffer, so you should not be running into that. how are you testing it? did you run yarn install?

@felipecsl
Copy link
Contributor Author

Great work investigating! Do you want to patch the version in this PR too to publish the fix?

Yup i'll bump the version and release

@felipecsl
Copy link
Contributor Author

ohh I see the error surfaces when you call init(), thanks @greghuels for pointing that out. fixup incoming

@greghuels
Copy link
Collaborator

how are you testing it? did you run yarn install?

I'm using the scripts/local-install.sh in this repo to install the js-client-sdk into a basic Next.js app I generated locally. I did yarn install, yes.

@felipecsl
Copy link
Contributor Author

Screenshot 2024-12-04 at 09 18 20

@felipecsl felipecsl requested a review from greghuels December 4, 2024 17:20
@felipecsl felipecsl changed the title fix: Set process.env.LOG_LEVEL to null in webpack config fix: Set process.env.LOG_LEVEL to null in webpack config + Buffer polyfill Dec 4, 2024
@felipecsl
Copy link
Contributor Author

Bumped shared sdk, retested everything, good to go!

// Make sure any usages of Buffer use the polyfill in the browser
new webpack.ProvidePlugin({
Buffer: ['buffer', 'Buffer'],
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is in theory no longer necessary now that we replaced Buffer with js-base64 in the common sdk, but I think it's worth leaving it here to cover our basis in case something else slips in the future

Copy link
Collaborator

@greghuels greghuels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it works now!

@felipecsl felipecsl merged commit 77501d8 into main Dec 4, 2024
8 checks passed
@felipecsl felipecsl deleted the felipecsl--process-env branch December 4, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants